-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(expect): add toBeOneOf
matcher
#6974
Conversation
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to discuss if this is actually worth it in builtin, but thanks for kicking it off!
One thing I wasn't sure is how error diff works. Can you add a test cases for that? You can find examples at the bottom of jest-expect.test.ts
using snapshotError
.
@hi-ogawa I added the snapshot tests |
I just found jest-extended provides |
That's a good idea. Just to be clear, you mean adding |
Kind of. If you use |
I added a example of adding |
toBeOneOf
matcher
I changed it to a custom matcher. Let me know if anything else should be changed. |
{ | ||
"actual": "0", | ||
"diff": "- Expected: | ||
toBeOneOf<Any,,> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff doesn't handle null
and undefined
correctly. It should be:
toBeOneOf<Any,,> | |
toBeOneOf<Any,null,undefined> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is unfortunate. Maybe can we try stringify
instead of String
? 🤔
vitest/packages/expect/src/jest-extend.ts
Lines 134 to 136 in 021944c
toAsymmetricMatcher() { | |
return `${this.toString()}<${this.sample.map(String).join(', ')}>` | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is better. But it also affects toSatisfy()
"value": toSatisfy<(value) => value % 2 !== 0>, | ||
"value": toSatisfy<[Function isOdd]>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It prints the function name instead of its implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is alright since this is how we normally format the function, so I guess we can say it's a better alignment.
`[AssertionError: expected Error: 2 to match object { message: toSatisfy{…} }]`, | ||
`[AssertionError: expected Error: 2 to match object { Object (message) }]`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this looks puzzling, but what's happening is we have truncation logic here for main error message and that just kicked in due to toSatisfy
format got a bit longer. This is fine as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
"value": toSatisfy<(value) => value % 2 !== 0>, | ||
"value": toSatisfy<[Function isOdd]>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is alright since this is how we normally format the function, so I guess we can say it's a better alignment.
`[AssertionError: expected Error: 2 to match object { message: toSatisfy{…} }]`, | ||
`[AssertionError: expected Error: 2 to match object { Object (message) }]`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this looks puzzling, but what's happening is we have truncation logic here for main error message and that just kicked in due to toSatisfy
format got a bit longer. This is fine as well.
Description
Following the discussion #6961 I'd suggest to add a new asymmetric matcher
expect.oneOf()
for better support of optional properties, which can often beundefined
ornull
if not set.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yaml
unless you introduce a new test example.Tests
pnpm test:ci
.Documentation
pnpm run docs
command.Changesets
feat:
,fix:
,perf:
,docs:
, orchore:
.